-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow use of the preferred_username OIDC claim. #1287
Conversation
9daec94
to
056c7ca
Compare
I think this looks sensible, the tests for oidc are located here: https://github.com/juanfont/headscale/blob/main/integration/auth_oidc_test.go Would be good if we could have one. |
Thanks. It looks like the current integration tests don't explicitly set claims, but the underlying mockoidc does let you queue up successive users with potentially different email/preferred_username claims. I'll try my hand at creating an integration test for this. |
Were you able to implement a testcase? I'd love to see this feature since it is blocking me from using OIDC for headscale. |
@vbrandl I'm working on it, but I'm new to Go and this codebase, and still figuring out how to more efficiently run the integration tests locally without just pushing to Github. The current OIDC tests don't actually check the claim responses at all, so I also need to thread those checks in. On my stuff, I'm running from these docker images if you want to try them out: https://github.com/meson800/headscale/pkgs/container/headscale |
I just tested your image and it does work as expected, but I'd prefer to run the official image for my infrastructure. So I'll wait until this gets merged. |
Thank you for this work @meson800 |
I did some testing on this and it appears to work great. I noticed the email is still cited on the "congrats you've registered your machine" page or whatever it is which might cause some confusion. That should probably also reflect the username if the setting is enabled. |
I'll look into changing that welcome page template @jonathanspw to respect the setting as well. On my local machine I have the mockoidc up and running with new mock users. this will be mergable hopefully within a week or so. |
6ed0b69
to
6fda090
Compare
@kradalby I added integration tests as requested, so this should be good for review. In sum, this PR includes:
Even with a full |
@@ -63,6 +65,29 @@ func mockOIDC() error { | |||
} | |||
accessTTL = newTTL | |||
} | |||
|
|||
mockUsers := os.Getenv("MOCKOIDC_USERS") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if this is prefixed with "HEADSCALE_INTEGRATION_"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is (technically) not just useful for integration tests, as this environment variable controls what mockoidc does if you e.g. set the env variable and ran headscale mockoidc
yourself. I followed the naming of the other variables in the mockOIDC
function (MOCKOIDC_CLIENT_ID
, MOCKOIDC_CLIENT_SECRET
, and so on). I can still prefix this if desired.
oidcMap := map[string]string{ | ||
"HEADSCALE_OIDC_ISSUER": oidcConfig.Issuer, | ||
"HEADSCALE_OIDC_CLIENT_ID": oidcConfig.ClientID, | ||
"CREDENTIALS_DIRECTORY_TEST": "/tmp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for Headscale or mockoidc? if its headscale, then prefix would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to CREDENTIALS_DIRECTORY_TEST
? I copied this environment map from the TestOIDCAuthenticationPingAll test. For whatever historical reason, that test passes in the client secret with a file instead of directly, which is the reason for the directory. I could simplify if needed (I don't see why CREDENTIALS_DIRECTORY_TEST
is a separate env variable).
Otherwise, looks great! |
I fixed the lint issues and updated the gold-standard YAML files to match the added configuration option. (and rebased on main again). |
Previously, Headscale would only use the `email` OIDC claim to set the Headscale user. In certain cases (self-hosted SSO), it may be useful to instead use the `preferred_username` to set the Headscale username. This also closes juanfont#938. This adds a config setting to use this claim instead. The OIDC docs have been updated to include this entry as well. In addition, this adds an Authelia OIDC example to the docs.
Updated the MockOIDC wrapper to take an environment variable that lets you set the username/email claims to return. Added two integration tests, TestOIDCEmailGrant and TestOIDCUsernameGrant, which check the username by checking the FQDN of clients. Updated the HTML template shown after OIDC login to show whatever username is used, based on the Headscale settings.
5d72a5b
to
a642383
Compare
Isn't it better if we add an internal mail domain for this case, i think this would be easier to deal with in the api and al taht stuff : ex: preferred_username : loprima would make an email like loprima@headscale.sso It would be more accurate with the Tailscale way of managing sso I guess |
I think that is something that should be up to your OIDC provider. If
you're self hosting, you can return those types of fake/internal email
addresses with the email grant.
Other self hostable software (Nextcloud, Matrix) deals with the possible
non-uniqueness of the preferred username claim by binding usernames to the
OIDC provider, which is something Headscale doesn't currently do. If
Headscale allows multiple OIDC providers in the future that will have ti be
fixed.
…On Thu, Apr 27, 2023, 3:14 PM loprima-l ***@***.***> wrote:
Isn't it better if we add an internal mail domain for this case, i think
this would be easier to deal with in the api and al taht stuff :
ex:
preferred_username : loprima would make an email like
***@***.***
It would be more accurate with the Tailscale way of managing sso I guess
—
Reply to this email directly, view it on GitHub
<#1287 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOEWD7LKA676I4VSPD4DO3XDLASHANCNFSM6AAAAAAWIJSWKI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I agree with that, but aren't we there to implement best practices to follow ? I think using multiple ways of storing a user "id" will be a mess that will complicate the interconnection between Headscale and other pieces of software. My advice is that we should have one way to identify a user, if we already have email based user distinction, wouldn't it be bad to implement other ways that doesn't fit with the Tailscale way ? |
The default OIDC settings prior to this PR already strips the domain from
the email claim unless you configure it not to. If you e.g. point Google's
OIDC provider at it, you'll get a Headscale user with @gmail.com removed.
There's still only one type of user ID with or without this PR.
I think there is an advantage for allowing the option of using the
preferred_username claim to create users; this is how OIDC supports letting
SSO users/administrators pick stable usernames despite changing a backing
email address. There is a unique source of truth on which user is which,
the OIDC provider.
I've only used Headscale; can you elaborate on what the Tailscale way to do
this is?
…On Thu, Apr 27, 2023, 3:31 PM loprima-l ***@***.***> wrote:
I agree with that, but aren't we there to implement best practices to
follow ? I think using multiple ways of storing a user "id" will be a mess
that will complicate the interconnection between Headscale and other pieces
of software.
My advice is that we should have one way to identify a user, if we already
have email based user distinction, wouldn't it be bad to implement other
ways that doesn't fit with the Tailscale way ?
—
Reply to this email directly, view it on GitHub
<#1287 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOEWDZVBYP5IX2JMG2UO3TXDLCR5ANCNFSM6AAAAAAWIJSWKI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Tailscale is using the provider domain, I think that's a better way to do it, maybe we must implement this way also in Headscale, but it would be necessary to use a headscale prefix, so we don't break up the compatibility with native Tascale APP, I don't know how they refer to the login methods. |
Thanks for the screenshot!
My feeling is that I think this probably belongs with multi-provider
support, not in this PR. I could work on it though in a future PR though.
I would imagine that with multiple providers, the headscale admin could
configure a suffix for each OIDC provider. E.g. Github could get a @github
prefix, self-hosted Authelia could get a @internal.sso prefix, whatever.
…On Thu, Apr 27, 2023, 4:03 PM loprima-l ***@***.***> wrote:
As you can see here : [image: See here]
<https://user-images.githubusercontent.com/69201633/234977645-4f4fbc59-d59e-465a-8bce-ca7594fb82f6.png>
Tailscale is using the provider domain, I think that's a better way to do
it, maybe we must implement this way also in Headscale, but it would be
necessary to use a headscale prefix, so we don't break up the compatibility
with native Tascale APP, I don't know how they refer to the login methods.
—
Reply to this email directly, view it on GitHub
<#1287 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOEWDZMQ6ZUBNQVGUSQWQLXDLGKXANCNFSM6AAAAAAWIJSWKI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I think that would be AMAZING <3 Like it will help a lot in the infinite quest of having a standardized system, I'm really busy to work on it but if you need help let me know I could write the docs if you want, I'm already planing to open a PR to add a gitbook. |
I merged against main so this can run tests again. @kradalby do you want me to change the environment variables as in the review above? I named them to be consistent with the surrounding code. @juanfont do you want me to update/edit this PR in some way/can you review? I see that there are other OIDC PRs that this might conflict with this (#1435), I can incorporate those changes if needed. |
When will this PR be merged to main branch, this feature is acutely I want. |
Yes, please. Immutable identifier to be used by headscale would be nice to have, as most IDPs allows users to change their emails, and thus break their headscale setup/login. |
I can try to update this branch to get it considered again; a lot has changed since I submitted the PR. |
Please do. |
Hi! as part of #1473, we have reorganised a lot of the code. To clear PRs that needs to be rebased or redone, we are closing open PRs that will require significant code change to be merged. In addition, the issue of the PR might in some cases have been fixed, change or no longer relevant, so it would be great if this is considered as well. Thank you for your contribution! If it is still relevant and the PR is reopened, we will aim at getting the changes into the next release after the reorg if accepted. |
@kradalby I can't reopen the PR myself, but this is ready to be considered again; I rebased and tested meson800:oidc_username_claim on top of the re-orged main. Summary
This PR adds the ability to use the The short version is that you can now set the Previous review commentsThere were some comments on prefixing some of the constants. I've kept it consistent (e.g. not prefixing the mockoidc constants with Integration testsIn particular, the integration tests are implemented by setting the mock OIDC user to Test failuresOf note, I ran all of the integration tests in a stub PR in my repo; some integration tests appear flaky so I had to rerun them, but one still fails despite reruns, Real testsI generated a Docker image for this PR and tested it out in my tailnet, and it works, correctly using the |
Looks awesome! Would really help my auth setup. |
I am using meson800's image and it works great. +1 to merge it! |
Any reason this wasn't merged? I see it's closed. I think it's a very welcome improvement. |
I think that one of the concerns is it wasn't generalizable enough. See this comment: #938 (comment) |
Previously, Headscale would only use the
email
OIDC claim to set the Headscale user. In certain cases(self-hosted SSO), it may be useful to instead use the
preferred_username
to set the Headscale username. This also closes the existing issue #938.This adds a config setting to use this claim instead. The OIDC docs have been updated to include this entry as well. In addition, this adds an Authelia OIDC example to the docs.
I didn't see any existing unit or integration tests for OIDC, so wasn't sure where to add such tests. If I'm wrong, I'm happy to add them!
I did test this against my SSO server, and confirm that the
preferred_username
grant if you set it works as expected.